I think #137 broke this. Again, not sure how to alert folks to the breaking change, @treydock you'll have to let me know how we publish those breaking changes in this module.

That PR fixed it so multiline strings actually worked. Before we were just passing \n characters as part of the single line string to OnDemand. I'm fairly certain that the old method would work still since strings are just strings when you do .to_yaml.

I'm fairly certain that the old method would work still since strings are just strings when you do .to_yaml.

We can test on our systems, but I'm fairly sure it doesn't work anymore or maybe we needed " instead of ' or something similar (\\n). If it worked, why did we change it?

In any case - I can submit patches tomorrow to our test systems to know for sure.

If it worked, why did we change it?

The classroom YAML was simply impossible with the old mechanism since the old mechanism was a bunch of ERB to render YAML so the classroom data would have been incredibly complex ERB and only solved that immediate problem and needed updates if the structure of our "custom" data changed. We don't need to constantly tweak ERB if we pass the entire data structure into to_yaml.

I also thought there was something changed with OnDemand so the newlines didn't work or something, maybe related to 3.1?

The unit tests still do the old method:

The contents are dumped:

$ bundle exec rspec spec/defines/cluster_spec.rb:49
Run options:
  include {:locations=>{"./spec/defines/cluster_spec.rb"=>[49]}}
  exclude {:bolt=>true}

openondemand::cluster
  when redhat-8-x86_64
---
v2:
  metadata:
    title: test
    hidden: false
  acls:
  - adapter: group
    groups:
    - test-group
    type: whitelist
  login:
    host: login.test
  batch_connect:
    basic:
      script_wrapper: module restore\n%s
    vnc:
      script_wrapper: module restore\nmodule load ondemand-vnc\n%s
    is expected to be nil

I don't think there is a way to force to_yaml to wrap strings with quotes but not sure that's entirely a problem:

If I dump the data from YAML.safe_load during unit tests:

{"v2"=>{"metadata"=>{"title"=>"test", "hidden"=>false}, "acls"=>[{"adapter"=>"group", "groups"=>["test-group"], "type"=>"whitelist"}], "login"=>{"host"=>"login.test"}, "batch_connect"=>{"basic"=>{"script_wrapper"=>"\"module restore\\n%s\""}, "vnc"=>{"script_wrapper"=>"module restore\\nmodule load ondemand-vnc\\n%s"}}}}

So if the escaped newline is what YAML.safe_load produces then maybe the thing consuming this needs updates to handle literal newlines that might be escaped? I don't think having the newline expanded and not escaped would work since that's not valid YAML since whitespace and newlines matter.

If it worked, why did we change it?

The classroom YAML was simply impossible...

No, I get why we changed this module's implementation - I meant why did we change our systems to use multi-line strings in merge request 601 on our internal puppet config repo.

I also thought there was something changed with OnDemand so the newlines didn't work or something, maybe related to 3.1?

I noticed this issue initially on our dev servers (when we were doing the classroom changes) and erroneously attributed it to 3.1 requiring a newer version of Ruby, so that's a red herring.

maybe the thing consuming this needs updates to handle literal newlines that might be escaped?

That's a good point. It's here -

https://github.com/OSC/ood_core/blob/122835c39d3a6115e43e78879b73805db54a660a/lib/ood_core/batch_connect/template.rb#L235

I think the change to multiline strings in OSC's Puppet code was to work around how to_yaml behaves different than what we did before with the complex ERB.

3.0.0 :006 > puts "module restore\\nmodule load ondemand-vnc\\n%s".to_s
module restore\nmodule load ondemand-vnc\n%s
 => nil
3.0.0 :007 > puts "module restore\nmodule load ondemand-vnc\n%s".to_s
module restore
module load ondemand-vnc
%s

I wonder if it's worth changing to this maybe?

3.0.0 :008 > puts "module restore\\nmodule load ondemand-vnc\\n%s".to_s.gsub('\n', "\n")
module restore
module load ondemand-vnc
%s
 => nil

Yea, to make matters worse - single quotes (what we had) behave differently.

irb(main):003:0> puts  'module restore\nmodule load ondemand-vnc\n%s'.to_s
module restore\nmodule load ondemand-vnc\n%s
=> nil
irb(main):004:0> puts  "module restore\nmodule load ondemand-vnc\n%s".to_s
module restore
module load ondemand-vnc
%s

I wonder if it's worth changing to this maybe?

Could be - at least on the script_wrapper anyhow. I'd have to wonder how safe it is, inevitably someone will have a \n literal they want to keep. I'll make a ticket on ood_core to investigate - but we'd likely want much stronger tests (i.e., tests that actually write files).

In any case - I think just documenting multi-line strings as the best practice solves a lot of these.